Skip to content

Conversation

@krichprollsch
Copy link
Member

@krichprollsch krichprollsch commented Jun 30, 2025

@krichprollsch krichprollsch self-assigned this Jun 30, 2025
@krichprollsch krichprollsch changed the title add pump message loop calls WIP add pump message loop calls Jul 1, 2025
@krichprollsch krichprollsch changed the title WIP add pump message loop calls add pump message loop calls Jul 1, 2025
}

pub fn pumpMessageLoop(self: *const Self) bool {
if (self.platform == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are potentially called in a tight loop, I think you can do this at comptime:

if (comptime builtin.is_test) {
    if (self.platform == null) return false;
}
// assume it's not-null in non-test.
return self.platform.?.inner.pumpMessageLoop(self.isolate, false);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if I should use a different slower loop for these tasks separated from the microtasks one.
WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this is doing, so it's hard to say.

The page's messageLoopCallback is not being called very often. all of page.navigate (including processing all synchronous scripts) are executed before it's ever called. This could change if navigate became fully asynchronous though.

It's only being called often when we're "done" navigating and waiting for
1 - New cdp sessions
2 - I/O (i.e. an XHR request is live)

In both cases, I would think it's mostly a no-op.

@karlseguin
Copy link
Collaborator

Besides the small [optional] optimization tweak above, PR is good.

Probably some opportunities to tighten up the test code. There's always a platform, but the fact that you don't have access to it in some parts of the test code is unfortunate. I think it's just that serveCDP that's the issue. Maybe we don't need those tests and instead rely on the integration / demo.

@krichprollsch
Copy link
Member Author

pump message loop could be relative to v8's garbage collector, but I'm not sure about that.
I suppose it's useful to run it time to time.
here we run it 1ms after page's initialize and then every 100ms. I think it's a good enough balance to start.

idk for idleTasks...

@krichprollsch krichprollsch merged commit f51ee7f into main Jul 3, 2025
10 checks passed
@krichprollsch krichprollsch deleted the pumpmessageloop branch July 3, 2025 17:11
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants